Skip to content

Cluster-Alias cleanup for VTOrc#11193

Merged
GuptaManan100 merged 3 commits intovitessio:mainfrom
planetscale:cluster-name-cleanup
Sep 12, 2022
Merged

Cluster-Alias cleanup for VTOrc#11193
GuptaManan100 merged 3 commits intovitessio:mainfrom
planetscale:cluster-name-cleanup

Conversation

@GuptaManan100
Copy link
Contributor

@GuptaManan100 GuptaManan100 commented Sep 9, 2022

Description

This PR addresses one of the long-standing cleanups required for VTOrc.
Orchestrator used to identify a cluster by the host:port name of the primary, which was used as the cluster_name. In order to have a more human-readable variant, it also had cluster_alias which could be set by regex matching from configurations, etc. Manual overrides could also be performed for a cluster_alias.

In the first iteration of VTOrc, an additional parameter called suggested_cluster_alias was added which was set to keyspace/shard.

Since VTOrc can just use this keyspace/shard to identify a cluster, it is much simpler to just use this for cluster_name and get rid of the other two fields. This leads to other cleanups too, where we don't have to change the cluster_name in case of a reparent, etc.

This PR makes this change, by cleaning up these two fields and setting cluster_name to keyspace/shard

Related Issue(s)

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@GuptaManan100
test: augment readTopologyInstanceBufferable test to expect the clust…
8ae707b 
…er name set to keyspace shard

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: remove cluster_alias and suggested_cluster_alias fields in favo…
c80209d 
…ur of a single cluster_name set to the keyspace/shard

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
feat: add tablet_type as a check for reading primaries
9d977a9 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100 GuptaManan100 added Type: Internal Cleanup Component: VTOrc Vitess Orchestrator integration labels Sep 9, 2022
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Sep 9, 2022

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive. Additionally, flag names should use dashes (-) as word separators rather than underscores (_).
  • If a workflow is added or modified, each items in Jobs should be named in order to mark it as required. If the workflow should be required, the GitHub Admin should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.

GuptaManan100
GuptaManan100 commented Sep 9, 2022
View reviewed changes
go/test/endtoend/vtorc/readtopologyinstance/main_test.go
assert.False(t, primaryInstance.HasReplicationCredentials)
assert.Equal(t, primaryInstance.ReplicationIOThreadState, inst.ReplicationThreadStateNoThread)
assert.Equal(t, primaryInstance.ReplicationSQLThreadState, inst.ReplicationThreadStateNoThread)
assert.Equal(t, fmt.Sprintf("%v:%v", keyspace.Name, shard0.Name), primaryInstance.ClusterName)
Copy link
Contributor Author

@GuptaManan100 GuptaManan100 Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This augments the existing ReadTopologyInstanceBufferable end to end test to also verify that we set the cluster_name correctly.

go/vt/orchestrator/app/cli.go
log.Fatal(err)
}
fmt.Println(instanceKey.DisplayString())
fmt.Println("Command deprecated")
Copy link
Contributor Author

@GuptaManan100 GuptaManan100 Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed some API and CLI commands that would have required some changes and that we don't intend to support anyways. I could have removed them as a separate PR, but I elected to do it here as I went along.

go/vt/orchestrator/inst/instance_dao.go
condition := `
cluster_name = ?
and (replication_depth = 0 or is_co_primary)
and tablet_type = ?
Copy link
Contributor Author

@GuptaManan100 GuptaManan100 Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This additional step is required because earlier when we started a new tablet, it used to come up and its cluster_name would be set to the hostname:port of itself. But now, it will be set the keyspace/shard and then that new tablet used to show up in this ReadClusterPrimary function.

A better way is to also employ filtering on the tablet_type to be Primary.

Currently this function isn't unit tested, but it is end-to-end tested in GracefulPrimaryTakeover tests. I intend to add unit tests for all these functions that are reading data from the backend, but it will require some testing code setup, so I have deferred it to another PR.

@GuptaManan100
Copy link
Contributor Author

GuptaManan100 commented Sep 9, 2022

Personally, I would have liked to go a step further and cleanup a few more things like make tablet_alias as the primary key everywhere instead of hostname,port and explicitly make cluster_name as two fields keyspace and shard to formalize it even further, but I will do that in separate PRs to keep them easy to review and test.

deepthi
deepthi approved these changes Sep 9, 2022
View reviewed changes
Copy link
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Thank you for the inline comments. They made this much easier to review.

@GuptaManan100 GuptaManan100 added Component: VTOrc Vitess Orchestrator integration and removed Component: VTOrc Vitess Orchestrator integration labels Sep 12, 2022
@GuptaManan100 GuptaManan100 merged commit a60f087 into vitessio:main Sep 12, 2022
@GuptaManan100 GuptaManan100 deleted the cluster-name-cleanup branch September 12, 2022 04:41
@shlomi-noach
Copy link
Contributor

shlomi-noach commented Sep 14, 2022

looks good to me!

@GuptaManan100 GuptaManan100 mentioned this pull request Sep 14, 2022
Merged
3 tasks
notfelineit pushed a commit to planetscale/vitess that referenced this pull request Sep 21, 2022
@GuptaManan100
Cluster-Alias cleanup for VTOrc (vitessio#11193) (vitessio#1055)
e3c897c 
* test: augment readTopologyInstanceBufferable test to expect the cluster name set to keyspace shard

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: remove cluster_alias and suggested_cluster_alias fields in favour of a single cluster_name set to the keyspace/shard

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add tablet_type as a check for reading primaries

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rsajwani rsajwani Awaiting requested review from rsajwani

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach shlomi-noach is a code owner

1 more reviewer

@deepthi deepthi deepthi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: VTOrc Vitess Orchestrator integration Type: Internal Cleanup

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GuptaManan100 @shlomi-noach @deepthi